I think we should move the duplicate check to the beginning of the AddSerializer method to implement fail-fast behavior. This would avoid unnecessary serializer transformation work when a serializer already exists for the node.

However, I notice the current implementation places this check at the end, likely to preserve information about the conflicting serializer for the exception. While this debugging information could be helpful, I question just how useful it is. Knowing where a duplicate attempt happened seems more useful knowing what the conflicting serializer would have been.

I lean toward the fail-fast approach since the conflicting serializer details seem like a "nice-to-have" rather than essential debugging information.

If we move the error check further up then we have to remove information about the redundant serializer from the error message.

Also, I don't think we need to worry about failing fast because we actually never expect to hit this error. This check is here to help us find bugs. In the absence of bugs this exception will never be thrown.

Seems like this is never used?

Removed.

Could we maybe make use of a state enum as such:

private enum DiscoveryPhase
{
    InferringFromContext,      // Not using defaults
    UsingDefaultSerializers,   // Last attempt with defaults
    Complete                    // No more progress possible
}

to replace the use of all these booleans used in keeping track of the state of the visitor?

It's somewhat reasonable, but I think the booleans work better here.

There are only 2 booleans.

One issue is that if we ever had more states the order of the enum values could get very confusing and maybe even no order is possible.

For net6 and higher there is a better way to check: both Tuple and ValueTuple implements ITuple interface. I suppose we can have #if def here and use interface approach for modern targets, when have something like the proposed code for net472.

Nice to know!

Is there any danger of false positives if unexpected classes also happen to implement ITuple?

Not doing just yet until we have a discussion about false positives.

Corrected spelling of 'DeduceSerialiers' to 'DeduceSerializers'.

Done

Corrected spelling of 'DeduceConditionaSerializers' to 'DeduceConditionalSerializers'.

Done

Corrected spelling of 'conversionsn' to 'conversions'.

Done

Corrected spelling of 'ExpressionWithUnknownSerialier' to 'ExpressionWithUnknownSerializer'.

Done

We usually create two strings depending on whether a feature is supported or not but, in this case, the MQL is so long it's easier to just remove limit : 1 if it is not supported.

Minor: I found only 2 usages of CanDeduceSerializer, both looks similar (TODO in the another usage says we have to check value types). I think we can simplify by refactoring to TryDeduceSerializer and do the type check and AddNodeSerializer inside it.

I've removed the CanDeduceSerializer method entirely becase there was another existing method that could be used.

It worth to add a comment why we decided stop looking for the serializer for the node.

Done

What the difference between IgnoreNodeSerializer and UnknowableSerializer?

IgnoreSerializer is when we know what the node means but there is no serializer needed for the node. But since we throw an exception if a node has no serializer we assign this dummy serializer to such nodes.

UnknowableSerializer is when we know what the node means in general, but not in this particular case. An example would be a method call expression with an unknown method (we know about method call expressions in general but can encounter methods we don't know anything about).

UnknowableSerializer comes into play with client-side projections. If we didn't assign the UnknowableSerializer dummy serializer to method call nodes we didn't know about we would throw a missing serializer exception which would prevent us from getting to the client-side projection.

initializers variable seems to be unused. Should we handle initializers somehow?

Not sure yet. Added a TODO comment for later investigation.

Is this TODO still actual?

Yes, but I don't know the full list.

I've edited the comment to not mention DateTime.Year (since that one is already done)

Let's move this method to DictionaryMethod helper class and avoid Contains("Dictionary") by checking for interface or base class.

Done.

I cannot find any usage of results of IsItemSerializerKnown method call results, so it probably could be removed. And if it's removed then outer if is redundant.

I've removed this one line.

I don't understand why you think the outer if is redundant.

We have number of exactly the same methods in this class to process all math trigonometry methods, should we group then into a single collection __trigonometricMathMethods and process in the single method like:

     void DeduceTrigonometryMethodSerializers()
        {
            if (method.Is(__trigonometricMathMethods))
            {
                DeduceReturnsDoubleSerializer();
            }
            else
            {
                DeduceUnknownMethodSerializer();
            }
        }

Good idea. Done.

This logic looks very similar to StandardSerializers.GetSerializer. Can we leverage it here?

Added a TODO for future investigation.

There are 11 types handled here that StandardSerializers doesn't know about and a few of the ones it does know about are configured differentlly here, so it's tricky.

Should we throw in default case?

We could, but I don't think it's necessary.

First of all, there are no other cases here AFAIK.

And if there are, it will result in a missing serializer exception a bit later on.

I don't think we normally throw exceptions when deducing things in the SerializerFinder. We either deduce something or we do nothing.

I've removed the CanDeduceSerializer method entirely becase there was another existing method that could be used.

Probably not until we add support for DateTimeOffset in general.

No tests are failing because we didn't do this.

Yes, but I don't know the full list.

I've edited the comment to not mention DateTime.Year (since that one is already done)

Yes this is surprising.

It's because Dictionary<TKey, TValue>.Keys and IDictionary<TKey, TValue>.Keys have different types. Same for Values.

An unexpected glitch in Microsoft's design of these types. (Constrained by having to maintain backward compatibility).

Moved to MathMethod.cs from where it can be shared.

Good idea. Done.

Added a TODO for future investigation.

There are 11 types handled here that StandardSerializers doesn't know about and a few of the ones it does know about are configured differentlly here, so it's tricky.

Done.

We could, but I don't think it's necessary.

First of all, there are no other cases here AFAIK.

And if there are, it will result in a missing serializer exception a bit later on.

I don't think we normally throw exceptions when deducing things in the SerializerFinder. We either deduce something or we do nothing.

IgnoreSerializer is when we know what the node means but there is no serializer needed for the node. But since we throw an exception if a node has no serializer we assign this dummy serializer to such nodes.

UnknowableSerializer is when we know what the node means in general, but not in this particular case. An example would be a method call expression with an unknown method (we know about method call expressions in general but can encounter methods we don't know anything about).

UnknowableSerializer comes into play with client-side projections. If we didn't assign the UnknowableSerializer dummy serializer to method call nodes we didn't know about we would throw a missing serializer exception which would prevent us from getting to the client-side projection.